Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #952 +/- ##
==========================================
- Coverage 75.69% 74.97% -0.73%
==========================================
Files 61 61
Lines 2786 2869 +83
==========================================
+ Hits 2109 2151 +42
- Misses 523 564 +41
Partials 154 154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I am traveling rn, will take a look probably Tuesday! |
This uses a single struct for the authentication. This prevents further re-requesting of the already requested data.
| public elevate = async (id: number, durationSeconds: number): Promise<void> => { | ||
| await axios.post(`${config.get('url')}client:elevate`, {id, durationSeconds}); | ||
| await this.refresh(); | ||
| this.snack('Client elevated'); |
There was a problem hiding this comment.
ux: when the selection is "cancel elevation" (if durationSeconds <0) it should show a different msg
| } | ||
|
|
||
| // RequireElevatedClient requires an elevated client token or basic auth. | ||
| func (a *Auth) RequireElevatedClient(ctx *gin.Context) { |
There was a problem hiding this comment.
nitpick: the helper function in auth package is getting more and more numerous and with cryptic boolean flag, maybe consider use some pattern (functional, etc) to compose things like "RequireAdmin", "RequireClient", "RequireElevatedClient", etc
|
|
||
| // RequireAdmin returns a gin middleware which requires a client token or basic authentication header to be supplied | ||
| // with the request. Also the authenticated user must be an administrator. | ||
| // RequireElevatedAdmin requires an elevated client token or basic auth, the user must be an admin. |
There was a problem hiding this comment.
The function name does not match doc name
| clientElevated := g.Group("") | ||
| { | ||
| clientElevated.Use(authentication.RequireElevatedClient) | ||
| clientElevated.POST("/client:elevate", clientHandler.ElevateClient) |
There was a problem hiding this comment.
Maybe /client/:id/elevate is more REST-y?
| client := auth.GetClient(ctx) | ||
| if client != nil { | ||
| result.ClientID = client.ID | ||
| result.ElevatedUntil = client.ElevatedUntil |
There was a problem hiding this comment.
I think it would make more sense and be more useful to hide this field if the server believes it is in the past. It gives a concreate yes or no for the client whether they are elevated right now irrespective of time differences.
| </TableCell> | ||
| <TableCell> | ||
| {elevatedUntil && Date.parse(elevatedUntil) > Date.now() ? ( | ||
| <TimeAgo date={elevatedUntil} formatter={TimeAgoFormatter.long} /> |
There was a problem hiding this comment.
Same concern as above, I would cap the granularity of this field to one minute because the "in 15 seconds" or "5 seconds ago" are false precision that may not be true depending on clock synchronization.
seems to be doable by setting a custom unit for react-timeago between minutes, hours and days
2 participants
step-up.webm
Fixes #944